src/tools/schemas.ts (1)

78-99: Consider explicit type handling over string preprocessing.

The preprocessing logic attempts to auto-parse JSON strings into arrays, which can mask type errors. If a client incorrectly passes a stringified JSON array when they should pass a proper array, this silently accepts it rather than surfacing the type mismatch.

Consider one of these approaches:

Option 1: Remove preprocessing and require proper typing from callers:

-  content: z.preprocess(
-    (val) => {
-      // If it's a string that looks like JSON array, parse it
-      if (typeof val === 'string' && val.trim().startsWith('[')) {
-        try {
-          return JSON.parse(val);
-        } catch {
-          // If parsing fails, return as-is (might be markdown content)
-          return val;
-        }
-      }
-      // Otherwise return as-is
-      return val;
-    },
-    z.union([z.string(), z.array(PdfOperationSchema)])
-  ),
+  content: z.union([z.string(), z.array(PdfOperationSchema)]),

Option 2: If preprocessing is needed for a specific client, at least log parse failures:

      if (typeof val === 'string' && val.trim().startsWith('[')) {
        try {
          return JSON.parse(val);
        } catch {
-          // If parsing fails, return as-is (might be markdown content)
+          // If parsing fails, return as-is (might be markdown content)
+          console.warn('Failed to parse JSON array in WritePdfArgsSchema content');
          return val;
        }
      }

src/tools/mime-types.ts (1)

5-7: Consider consistent handling pattern for MIME types.

The PDF extension uses an early return while image types use a dictionary lookup. For consistency and maintainability, consider using the same pattern for all MIME type mappings.

Apply this diff for consistency:

 export function getMimeType(filePath: string): string {
   const extension = filePath.toLowerCase().split('.').pop() || '';
 
-  if (extension === "pdf") {
-    return "application/pdf";
-  }
-
   // Image types - only the formats we can display
   const imageTypes: Record<string, string> = {
     'png': 'image/png',
     'jpg': 'image/jpeg',
     'jpeg': 'image/jpeg',
     'gif': 'image/gif',
     'webp': 'image/webp'
   };
 
+  // Document types
+  const documentTypes: Record<string, string> = {
+    'pdf': 'application/pdf'
+  };
+
+  if (extension in documentTypes) {
+    return documentTypes[extension];
+  }
+
   // Check if the file is an image
   if (extension in imageTypes) {
     return imageTypes[extension];
   }

src/tools/pdf/markdown.ts (1)

9-16: Consider consistent return type for loadPdfToBuffer.

The function returns Buffer for local files and ArrayBuffer for URLs, requiring the caller to handle both. Consider normalizing to a single type (e.g., always return Uint8Array or Buffer).

Apply this diff to return consistent type:

-async function loadPdfToBuffer(source: string): Promise<Buffer | ArrayBuffer> {
+async function loadPdfToBuffer(source: string): Promise<Uint8Array> {
     if (isUrl(source)) {
         const response = await fetch(source);
-        return await response.arrayBuffer();
+        const arrayBuffer = await response.arrayBuffer();
+        return new Uint8Array(arrayBuffer);
     } else {
-        return await fs.readFile(source);
+        const buffer = await fs.readFile(source);
+        return new Uint8Array(buffer);
     }
 }

Then simplify the caller:

 export async function parsePdfToMarkdown(source: string, pageNumbers: number[] | PageRange = []): Promise<PdfParseResult> {
     try {
         const data = await loadPdfToBuffer(source);
 
-        // @ts-ignore: Type definition mismatch for ESM usage
-        return await pdf2md(new Uint8Array(data), pageNumbers);
+        return await pdf2md(data, pageNumbers);
 
     } catch (error) {

src/tools/filesystem.ts (4)

331-347: PDF handling from URL reads entire document regardless of offset/length.

The URL-based PDF reading (readFileFromUrl) doesn't support offset and length parameters for page filtering, unlike readFileFromDisk which passes these to parsePdfToMarkdown. This is inconsistent with the documented behavior that PDFs support page filtering.

Consider passing offset/length if the function signature is expanded, or document this limitation explicitly in the tool description.

---

1207-1214: Avoid mutating input operation objects directly.

The code mutates the original operation objects (o.sourcePdfPath = await validatePath(...)) which could cause unexpected side effects if the caller reuses the operations array. Consider creating new operation objects instead.

         for (const o of content) {
             if (o.type === 'insert') {
                 if (o.sourcePdfPath) {
-                    o.sourcePdfPath = await validatePath(o.sourcePdfPath);
+                    operations.push({
+                        ...o,
+                        sourcePdfPath: await validatePath(o.sourcePdfPath)
+                    });
+                    continue;
                 }
             }
             operations.push(o);
         }

---

1182-1183: Consider stronger typing for the options parameter.

The options parameter is typed as any, which bypasses TypeScript's type checking. Consider using a more specific type or at least Record<string, unknown> for better type safety.

---

915-922: Redundant isPdfFile check.

Line 915 calls isPdfFile(fileResult.mimeType) but the result is never used. The fileResult.isPdf property (line 921) is already set by readFile, making this check redundant.

-                const isPdf = isPdfFile(fileResult.mimeType);
                 return {
                     path: filePath,
                     content: typeof fileResult === 'string' ? fileResult : fileResult.content,
                     mimeType: typeof fileResult === 'string' ? "text/plain" : fileResult.mimeType,
                     isImage: typeof fileResult === 'string' ? false : fileResult.isImage,
                     isPdf: typeof fileResult === 'string' ? false : fileResult.isPdf,
                     payload: typeof fileResult === 'string' ? undefined : fileResult.payload
                 };

src/tools/pdf/manipulations.ts (1)

70-70: Use strict equality operator.

Line 70 uses == instead of === for the type check. For consistency with line 67 and JavaScript best practices, use strict equality.

-        else if (op.type == 'insert') {
+        else if (op.type === 'insert') {

src/tools/pdf/lib/pdf2md.ts (1)

107-122: Unusual try/finally pattern with return inside try.

The pattern of having return inside a try block with a finally for cleanup is functional (JavaScript guarantees finally runs), but it's unconventional and could confuse readers. A more common pattern:

-    try {
-        return { pages: processedPages, metadata };
-    } finally {
+    const result = { pages: processedPages, metadata };
+    
+    // Cleanup
+    try {
         if (pdfDocument) {
-            try {
-                if (typeof pdfDocument.cleanup === 'function') {
-                    await pdfDocument.cleanup(false);
-                }
-            } catch (e) { }
-            try {
-                if (typeof pdfDocument.destroy === 'function') {
-                    await pdfDocument.destroy();
-                }
-            } catch (e) { }
+            if (typeof pdfDocument.cleanup === 'function') {
+                await pdfDocument.cleanup(false);
+            }
+            if (typeof pdfDocument.destroy === 'function') {
+                await pdfDocument.destroy();
+            }
         }
-    }
+    } catch (e) { /* Ignore cleanup errors */ }
+    
+    return result;

However, the current code is functionally correct.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/tools/pdf/extract-images.ts (1)

93-93: Misleading comment on compressedSize calculation.

The calculation Math.round(compressionResult.data.length * 0.75) correctly estimates the binary size from the base64 string, but the comment "Approximate base64 overhead" is misleading. This is actually computing the approximate original binary size, not the overhead.

-                                    compressedSize: Math.round(compressionResult.data.length * 0.75) // Approximate base64 overhead
+                                    compressedSize: Math.ceil(compressionResult.data.length * 3 / 4) // Approximate binary size from base64

src/handlers/filesystem-handlers.ts (1)

56-67: Configured default read limit may still be ignored if Zod sets a length default.

You derive defaultLimit from config.fileReadLineLimit, but then always use parsed.length ?? defaultLimit. If ReadFileArgsSchema still defines length with a Zod .default(...), parsed.length will always be set and the config-driven limit will never be used, which is the same issue flagged in the earlier review.

Consider detecting whether length was actually provided on the raw args and only using parsed.length in that case; otherwise, fall back to the config limit:

const rawArgs = args as { offset?: number; length?: number } | undefined;

// Use caller‑provided values when present; otherwise, fall back to config defaults.
const offset = rawArgs && 'offset' in rawArgs ? parsed.offset : 0;
const length = rawArgs && 'length' in rawArgs ? parsed.length : defaultLimit;

This keeps the nice Zod parsing while making fileReadLineLimit effective again.

src/tools/pdf/extract-images.ts (1)

170-170: Consider validating channels before type assertion.

The type assertion channels as 1 | 2 | 3 | 4 assumes unpdf always returns a valid channel count. If unpdf returns an unexpected value (e.g., 0 or 5), sharp may throw a cryptic error. Since sharp's error handling is already in a try/catch, this is low risk, but a validation could provide clearer diagnostics.

+        // Validate channel count for sharp compatibility
+        if (channels < 1 || channels > 4) {
+            console.warn(`Unsupported channel count: ${channels}`);
+            return null;
+        }
+
         let pipeline = sharp(Buffer.from(data), {
             raw: {
                 width,
                 height,
                 channels: channels as 1 | 2 | 3 | 4
             }
         });

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/handlers/filesystem-handlers.ts (1)

166-179: Major: Missing optional chaining on forEach calls can cause runtime errors.

Lines 167-168 still have the bug flagged in the past review: result.payload?.pages.forEach(...) and page.images.forEach(...) will throw a TypeError if pages or images is undefined, because optional chaining (?.) only protects the property access, not the method call.

Apply this diff to add optional chaining on both forEach calls:

             if (result.isPdf) {
-                result.payload?.pages.forEach((page, i) => {
-                    page.images.forEach((image, i) => {
+                result.payload?.pages?.forEach((page) => {
+                    page.images?.forEach((image) => {
                         contentItems.push({
                             type: "image",
                             data: image.data,
                             mimeType: image.mimeType
                         });
                     });
                     contentItems.push({
                         type: "text",
                         text: page.text,
                     });
                 });

Based on the past review comment.

src/tools/pdf/markdown.ts (1)

18-21: Import PageRange from pdf2md instead of re‑declaring it

PageRange is already defined and exported in src/tools/pdf/lib/pdf2md.ts. Duplicating the type here risks the two definitions drifting over time and breaks the “single source of truth” for the paging contract. Better to import the shared type and delete this local alias.

You can do something like:

-import { PdfParseResult, pdf2md } from './lib/pdf2md.js';
+import type { PdfParseResult, PageRange } from './lib/pdf2md.js';
+import { pdf2md } from './lib/pdf2md.js';
@@
-type PageRange = {
-    offset: number;
-    length: number;
-}

This keeps the runtime import for pdf2md intact while reusing the canonical PageRange type.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro